Aspects that I suspect are just bad:

• GetRange - not exactly a fitting name 🤷
• get_byte_ranges - no longer Vec wrt get_bytes. Intended to avoid dealing with throwing out GetRange::Suffix before passing into ObjectStore::get_ranges / reimplementing that.

Aspects that need consensus:

• which of the two options (suffix, or non-suffix) gets the load method name (i.e. which is the default), and what the non-default method name should be.
• How explicit should switching on (or if it's the default, off) suffix requests be?

@H-Plus-Time do you need any help with this? Or is this at a state where it needs API decisions from maintainers?

@H-Plus-Time do you need any help with this? Or is this at a state where it needs API decisions from maintainers?

Yep, the main one I suppose is whether Offset ranges are ever used in parquet readers - if they're not, there's a few variants and optionals we can throw out.

Other than that, naming and the get_bytes vs get_byte_ranges deviations.

Maybe it's worth taking this out of draft mode then?

I'm pretty apprehensive about changing the default behaviour to use suffix requests given there are stores that are known to not support them.

I wonder what you think of instead performing the initial suffix read of the prefetch size manually, and then constructing an AsyncFileReader with these bytes cached? This initial fetch request would tell you the total size of the object and so no changes would be necessary to traits, allowing us to keep the current simple interface and avoid potential compatibility footguns, but also allow you to avoid making additional HEAD requests for your use case?

I'm pretty apprehensive about changing the default behaviour to use suffix requests given there are stores that are known to not support them.

I wonder what you think of instead performing the initial suffix read of the prefetch size manually, and then constructing an AsyncFileReader with these bytes cached? This initial fetch request would tell you the total size of the object and so no changes would be necessary to traits, allowing us to keep the current simple interface and avoid potential compatibility footguns, but also allow you to avoid making additional HEAD requests for your use case?

I agree with defaulting to non-suffix requests (consensus seems to be reached on that).

The manual approach isn't an option in my main use-case (browser side fetch as the underlying IO method) - usually resources are cross-origin, and Content-Range is not CORS-safelisted (the target origin must include it in Access-Control-Expose-Headers), so you can't determine the object's size from a ranged GET request alone without considerable intervention.

Shifting the suffix request path to a secondary load_without_size, and probably re-emphasising the file_size parameter (that is, make it an explicit choice on the users' part to not give the reader that information), I think that would be the least disruptive change.

Perhaps the following would be sufficient:

/// store.rs
impl ParquetObjectReader {
  pub fn new(store: Arc<dyn ObjectStore>, meta: ObjectMeta) -> Self
  pub fn new_without_size(store: Arc<dyn ObjectStore>, location: Path) -> Self
}

/// metadata.rs

impl<F: MetadataFetch> MetadataLoader<F> {
  pub async fn load(mut fetch: F, file_size: usize, prefetch: Option<usize>) -> Result<Self>
  pub async fn load_without_size(mut fetch: F, prefetch: Option<usize>) -> Result<Self>
}

// fetch_parquet_metadata elided for brevity - file_size: usize -> Option<usize>

With the above, the remaining footguns (calling fetch_parquet_metadata with file_size=None, calling load_without_size, or supplying a suffix range directly to get_bytes) would require quite a lot of intent.

Can you not add the header to https://docs.aws.amazon.com/AmazonS3/latest/userguide/ManageCorsUsing.html#cors-expose-headers. Given you have to configure CORS for anything to work, it seems requiring an additional header isn't that burdensome?

These are typically origins not controlled by the requester, configured with boilerplate CORS - unlikely to ever change.

So long as we don't break existing workloads, I don't see an issue. However, I will observe that the code commonality between a suffix and range based MetadataLoader will likely be relatively small... Rather than adding code to support what is a fairly niche use-case, it might be worth this living in a repository dedicated to WASM support? Given the first-party S3 implementation doesn't support WASM32, let alone CORS-filtered responses that are missing expected headers, I suspect you must be rolling your own anyway?

I don't quite understand why non-WASM APIs wouldn't want first-class suffix-based loading support as well? I'm building APIs for both WASM/JS and Python, and in Python it seems a suffix-based loading approach would always be preferable when the user passes in a string URL or object store path (whenever the API doesn't already have the content length).

• For all object stores that support suffix requests, we save one request by getting the content length and metadata length at the same time.
• In the Azure case that doesn't support suffix requests, the ObjectStore fallback does a head then range, which is no worse than now whenever the ObjectMeta is not already known.

I don't quite understand why non-WASM APIs wouldn't want first-class suffix-based loading support as well

This is already supported as I articulated above - #6157 (comment), WASM is a particular unique case where CORS enforcement strips the Content-Range header from the response

Downstream consumers in Rust or Rust+PyO3 using the cache-populating approach you (@tustvold) suggested would still need to roll their own cache, right?

As for the wasm32 usecase - yes, the object_store implementation(/s) are 3rd party, but not the parquet machinery. The volume of code required to do what this PR proposes externally is significant - a full clone of ParquetObjectReader, more or less full reimplementation of the MetadataLoader (due to a combination of remainder being (correctly) private, and the bumpalloc-like nature of wasm's linear memory).

Right, I'll try to find some time to work out a better way to proceed here over the next few days. It seems unfortunate to add suffix fetches to AsynFileReader when they're only applicable to metadata fetching, which is already a separate method

Edit: see also #6002 for some other discussion on parquet metadata

I am depressed about the large review backlog in this crate. We are looking for more help from the community reviewing PRs -- see #6418 for more

Marking as draft as we don't think this one is waiting for feedback anymore

The latest update was from @tustvold:

Right, I'll try to find some time to work out a better way to proceed here over the next few days.

I'm not sure if there's other work to do before a re-review of this PR.

It may be that some of this PR has been superseded by all the progress on the ParquetMetaDataReader?

It may be that some of this PR has been superseded by all the progress on the ParquetMetaDataReader?

sadly not :( I deferred the suffix reading, hoping for a solution here first. ParquetMetaDataReader still needs the file size to do async reads.

I'm afraid I've not had time to look into this nor to follow the recent changes to parquet metadata loading. I suspect what needs to happen is for someone to find a way to incorporate this into the new ParquetMetaDataReader, but I'll confess to not really knowing what that might look like.

Ideally we can find a way to share code effectively, whilst also not altering the current behaviour. I would be less excited about an approach that simply creates an entirely new codepath